Add ambiguity expansion iterator #46
Conversation
src/expansions.rs
Outdated
| }; | ||
|
|
||
| for amb in self.ambiguities.iter_mut().rev() { | ||
| amb.digit = (amb.digit + 1) % (amb.nucleotide.possibilities().len() as u8); |
There was a problem hiding this comment.
I wonder if it makes sense (and if it even makes a performance difference) to make a method on nucleotides that immediately returns the number of possibilities, rather than write .possibilities().len() (there are a few occurrences elsewhere in the PR).
Also I really like this algorithm :)
There was a problem hiding this comment.
I'm thinking the implementation can be usize::count_ones(self.into()) because of our chosen encoding of ambiguous nucleotides, which is often a single operation (popcnt in x86).
There was a problem hiding this comment.
Oh! That's an even better idea. I'll try that.
There was a problem hiding this comment.
I tried this out on my laptop with no noticeable difference, so I was about to discard that change... but just to be diligent I tried it on biofilter and it consistently yielded a whopping 15-20% performance improvement! I really need to be better about doing performance testing on biofilter instead of my dev machine.
Replaces `nuc.possibilities().len()` with `nuc.bits().count_ones()`.
Intent
If we're going to support wobble-codes we'll need some kind of support for expansions. Instead of using the pipeline's expansion code, this uses an iterator-based approach because that allows calculating the number of expansions without actually enumerating each one, so the API doesn't need an expansion cap to be specified upfront. Additionally, the included benchmarks show that switching to an iterator-based approach shouldn't cause any performance problems.
Changes
quickdna::expansions::{Expansion, Expansions}.DnaSequenceAmbiguous::expansions.